Add OpenHouseViewCatalog (iceberg-1.5) with mock backend to prototype view create/read - #655
Add OpenHouseViewCatalog (iceberg-1.5) with mock backend to prototype view create/read#655aastha25 wants to merge 2 commits into
Conversation
…totype) Adds a 1.5-only OpenHouseViewCatalog that extends OpenHouseCatalog (tables inherited unchanged) and implements Iceberg's org.apache.iceberg.catalog.ViewCatalog, delegating view ops to a composed InMemoryCatalog mock. This exercises the create/read view plumbing before the OpenHouse Views service and its generated client exist. Lives in the iceberg-1.5 source set so the Spark 3.1 / Iceberg 1.2 runtime stays table-only. Co-authored-by: Copilot <[email protected]>
| // CREATE VIEW works without a prior CREATE NAMESPACE (real backend will own this policy). | ||
| Namespace namespace = identifier.namespace(); | ||
| if (!mockViewCatalog.namespaceExists(namespace)) { | ||
| mockViewCatalog.createNamespace(namespace); | ||
| } |
There was a problem hiding this comment.
Should we prioritize database level abstraction in openhouse for views?
There was a problem hiding this comment.
Agreed that we should prioritize the database-level abstraction for views regardless. Having said that, we likely don't need net-new database machinery for the initial setup and views can reuse the exact db provisioning model tables already follow.
| * adopted to minimize any interference with production code paths. | ||
| */ | ||
| @Slf4j | ||
| public class OpenHouseViewCatalog extends OpenHouseCatalog implements ViewCatalog { |
There was a problem hiding this comment.
Assuming this will replace Conventional OH catalog? Why not directly extend the conventional one to implement ViewCatalog to maintain backward compatibility and avoid potential drift between adopting table vs view catalog?
There was a problem hiding this comment.
the existing OpenHouseCatalog (table catalog) is used in Spark 3.1 with Iceberg 1.2, and this prototype only enables views on Spark 3.5 / Iceberg 1.5, hence the extension + isolation. It'll be an easy swap to fold ViewCatalog into OpenHouseCatalog directly if & when we extend support to the 3.1 branch.
There was a problem hiding this comment.
I highly suggest that we just target 3.5 first and get it solid before expanding the surface area to 3.1 / iceberg1.2.
| // OpenHouseCatalog.newTableOps() returns OpenHouseTableOperations calling TableApi. Only this | ||
| // field's type + init change; the ViewCatalog delegation below stays identical. | ||
| // TODO(views-prototype): swap mock -> OH Views service client once the server API + ViewApi land. | ||
| private final InMemoryCatalog mockViewCatalog = new InMemoryCatalog(); |
There was a problem hiding this comment.
One approach to make this PR production ready is to move the InMemoryCatalog to test classes and keep the APIs here.
| * asViewCatalog} (it only enables views when the configured {@code catalog-impl} is {@code | ||
| * instanceof ViewCatalog}). | ||
| * | ||
| * <p>Because it already {@code extends OpenHouseCatalog} (for tables), Java single inheritance |
There was a problem hiding this comment.
Can we do this via composition vs inheritance?
There was a problem hiding this comment.
Assuming you're asking - can OpenHouseViewCatalog hold OpenHouseCatalog (composition) instead of extend it? Yes, thats possible, OHViewCatalog could implement the table operations and forward those reqs to a composed OpenHouseCatalog . Downsides are that (1) we hand-forward the entire table + namespace + grant surface, and (2) any new method added to OpenHouseCatalog has to be wired up explicitly in OpenHouseViewCatalog whereas in the current approach, we get it for free and its clean.
If we dont foresee a lot of damage from (2), we could do the composition & OHViewCatalog extends BaseMetaStoreViewCatalog. But, that also adds a lot of biolerplate code to wire up its (generic) table ops back to OH's REST table logic.
There was a problem hiding this comment.
we can roll with the existing layout and revisit soon-ish if composed viewCatalog turns out to be tricky / unclean
There was a problem hiding this comment.
Thanks for the prototype.
In production today we already run two catalogs with different responsibilities:
spark.sql.catalog.openhouse.catalog-impl=com.linkedin.openhouse.spark.LiOpenHouseCatalog
spark.sql.catalog.spark_catalog=com.linkedin.coral.spark.catalog.CoralSparkViewCatalogThe issue is that Coral catalog has more logic than the OH catalogs, and the divergence has been a lasting production/compliance hole. reconciliation of 2 into 1 is high LoE and high risk (caused one outage) and is stalled. The testing across both catalog's diverges so changes in 1 can break 2 like a CTAS doesn't carry nullability bug OH fixed last year.
OpenHouseViewCatalog (and the likely follow-on LiOpenHouseViewCatalog) creates a third catalog type at the Iceberg layer:
OpenHouseCatalog
├── LiOpenHouseCatalog # prod tables
└── OpenHouseViewCatalog # this PR
└── LiOpenHouseViewCatalog? # likely LI fork
+ CoralSparkViewCatalog # prod views (already diverged)Also, Coral handles views for spark_catalog. This PR makes openhouse handle views too. Two paths = two places to keep in sync = another divergence problem.
TLDR; The primary issue is three parallel production catalogs with diverging policies and implementation.
and the ask: don't land this as a new long-lived catalog type on main yet.
Preferred paths:
- Fold into the existing catalog: implement ViewCatalog on OpenHouseCatalog for the Spark 3.5 / iceberg-1.5 line, instead of a parallel OpenHouseViewCatalog (and a future LiOpenHouseViewCatalog).
- Or keep the prototype in test code: if the goal is just create/read plumbing, put the mock in test sources / a test module so we don't ship InMemoryCatalog as a production catalog-impl.
- Otherwise, pause on merging until we decide the Coral relationship in the implementation: either OH becomes Coral’s view backend, or OH replaces Coral for OH-native views with a clear deprecation path.
|
Thanks @cbb330 , this is the right frame, and I share the core concern. Catalog unification has been top-of-mind for the group and me for a while; we're not looking to add a third top-level catalog to manage, and I'm tracking the unification work with tickets so this doesn't harden into a permanent parallel catalog. This is a gated alpha, not a new user-facing production catalog today, but it's also the direction and here's how it converges. (1) Default catalog stays table-only. The production Mapping to your three suggestions: Ask: you've made real strides with the |
Summary
Adds
OpenHouseViewCatalog, a view-capable catalog that extendsOpenHouseCatalog(tables inherited unchanged) and implements Iceberg'sorg.apache.iceberg.catalog.ViewCatalog, delegating view ops to a composed in-memory mock. This exercises the view create/read plumbing before the OpenHouse Views service and its client exist. It lives only in the iceberg-1.5 source set, so the Spark 3.1 / Iceberg 1.2 runtime stays table-only.Changes
For all the boxes checked, please include additional details of the changes made in this pull request.
✓ New Features
• New class
com.linkedin.openhouse.javaclient.OpenHouseViewCataloginintegrations/java/iceberg-1.5/openhouse-java-runtime(iceberg-1.5-only source set).• implements
ViewCatalogand delegatesbuildView/loadView/listViews/dropView/renameViewto a composedInMemoryCatalog(mock, ephemeral, no persistence).• RESTCatalog-style delegate-and-implement (not extends
BaseMetastoreViewCatalog) since it already extendsOpenHouseCatalog. The mock is a placeholder for a future OpenHouse Views-service-backed newViewOps() -> ViewApi , mirroring OpenHouseCatalog -> TableApi .• No changes to shared iceberg-1.2 sources; no gradle changes.
Testing Done
For all the boxes checked, include a detailed description of the testing done for the changes made in this pull request.
✓ No tests added or updated.
• Prototype/scaffolding with a mock backend; no OpenHouse service or wiring yet, so no meaningful unit/integration surface to assert. Verified compileJava , checkstyleMain , and spotbugsMain pass for both iceberg-1.5 and iceberg-1.2 runtimes (the latter confirms Spark 3.1 stays unaffected). Manually validated buildView(...).create() → loadView(...) round-trip via spark-shell against a locally published snapshot.
Additional Information
For all the boxes checked, include additional details of the changes made in this pull request.
• Breaking Changes — none (purely additive; new class, no existing code paths touched).
• Deprecations — none.